Skip to content

feat(pin): add step progress bar to PIN setup flow#115

Open
subinita01 wants to merge 3 commits into
odudex:masterfrom
subinita01:feat/pin-setup-progress-bar
Open

feat(pin): add step progress bar to PIN setup flow#115
subinita01 wants to merge 3 commits into
odudex:masterfrom
subinita01:feat/pin-setup-progress-bar

Conversation

@subinita01

@subinita01 subinita01 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

@odudex ,
Adds theme_create_progress_bar(), a slim lv_bar indicator anchored below a page title, and wires it into pin_page.c's build_chrome() so the 4-step first-time PIN setup (choose, confirm, split, show words) shows current progress. Gated on current_mode != PIN_PAGE_UNLOCK so it never appears on unlock or the delay/wipe screen.

Adds theme_create_progress_bar(), a slim lv_bar indicator anchored below
a page title, and wires it into pin_page.c's build_chrome() so the
4-step first-time PIN setup (choose, confirm, split, show words) shows
current progress. Gated on current_mode != PIN_PAGE_UNLOCK so it never
appears on unlock or the delay/wipe screen.
@subinita01 subinita01 marked this pull request as ready for review July 4, 2026 18:13
Narrow the bar from 60% to 40% width and thin its track to match, so
it reads as a subtle indicator rather than competing with the title.
@odudex

odudex commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR! The progress bar looks good and the fundamentals check out: step numbering is correct on every screen (including the deferred show-words build), bars don't accumulate across state transitions since lv_obj_clean() runs before each rebuild, and the layout works on all board sizes. A few things to address before merge:

1. The flow can end at step 3 of 4 (pin_page.c:381)

When eFuse provisioning is declined by the user, fails, or pin_efuse_check() returns PIN_EFUSE_ERROR, the PIN is saved directly from the split screen (split_confirm_cb at pin_page.c:700-706, efuse_confirm_result at pin_page.c:796) and STATE_SETUP_SHOW_WORDS never appears. Declining the irreversible eFuse burn is a routine choice, so on those devices the bar promises a 4th step that never comes, on first setup and on every subsequent change-PIN flow. Either make the total dynamic based on whether show-words will run, or drop the show-words step from the bar entirely.

2. Redundant mode gate (pin_page.c:403)

The current_mode != PIN_PAGE_UNLOCK check is dead code: every state reachable in UNLOCK mode returns 0 from setup_step_for_state() (the unlock→setup transition at pin_page.c:261-262 only fires in CHANGE mode), so if (step > 0) alone decides visibility in all three modes. In CHANGE mode the unlock/delay screens pass the mode check anyway and only the step check suppresses the bar, so the comment "never shown during unlock" describes the wrong mechanism. Please drop the mode check and keep just the step check.

3. Step count can silently drift (pin_page.c:381)

PIN_SETUP_STEP_COUNT and the setup_step_for_state() switch have no compile-time link, and lv_bar silently clamps values above the max, so inserting a setup screen and forgetting the define would render a full bar on the last two screens with no build error. Suggest deriving the total from the switch itself, e.g. setup_step_for_state(STATE_SETUP_SHOW_WORDS), so there's one source of truth.

4. Minor: NULL-anchor fallback in theme_create_progress_bar (theme_widgets.c:222)

The anchor == NULL branch is currently unreachable (the only caller always passes the title), and if it were used, theme_default_padding() from the top lands inside the corner-button band that theme_create_page_title deliberately avoids. Fine to keep the tolerant parameter, but the fallback position should account for the button band, or just require a non-NULL anchor.

5. Question: change-PIN flow shows "n of 4" across 5 screens

In PIN_PAGE_CHANGE mode the user sees 5 screens (verify current PIN + the 4 setup screens) but the bar only counts the setup portion. Treating the verify screen as a gate rather than a step is a reasonable design, just confirming it's intentional.

@subinita01

Copy link
Copy Markdown
Contributor Author

Thank you😊 for the detailed review @odudex.

For 1 & 3 -- I'll compute the total step count dynamically based on eFuse availability instead of hardcoding 4. STATE_SETUP_SHOW_WORDS will either not count as a step, or the total will drop to 3 when that path is skipped.

For 2 -- I'll remove the PIN_PAGE_UNLOCK guard so the bar appears correctly in change-PIN mode too.

For 4 -- I'll require a non-NULL anchor (since the only caller always passes the title) and add an assertion to make that explicit.

For 5-- Yes, intentional , the verify screen in change-PIN mode is a gate, not a setup step. The bar only counts the steps where the user is constructing the new PIN.

✌️🙌🤞🤞✌️

@subinita01

subinita01 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@odudex , Please check the updated PR

Drop STATE_SETUP_SHOW_WORDS from the step count since it's only
reached when eFuse provisioning is accepted and succeeds, so the
bar no longer promises an unreachable 4th step when it's declined
or fails. Replace the disconnected #define + switch with an enum
so the total is derived from the same switch that assigns steps.

Remove the redundant PIN_PAGE_UNLOCK mode guard in build_chrome:
every unlock/delay/wipe state already maps to 0, so step > 0 was
already doing all the work.

Require a non-NULL anchor in theme_create_progress_bar via
LV_ASSERT_NULL instead of a silent NULL-tolerant fallback, since
the only caller always passes the page title and the fallback's
offset landed inside the corner-button band.
@subinita01 subinita01 force-pushed the feat/pin-setup-progress-bar branch from 719d014 to be21307 Compare July 9, 2026 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants